Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure each symbol address doesn't start with a jump instruction #384

Closed
wants to merge 1 commit into from
Closed

Conversation

cormander
Copy link
Contributor

This patch makes kpatch_link_object run through all the verify steps before it attempts to patch them. On failure, there is no need to call unlink (although unlink is called on the return path anyway - but I'll tackle that problem another day) so add the err_verify jump point.

We also now check the start of the symbol address for the 0xe9 OP_JMP_REL32 instruction. This may not be the best place to do it, but it's the soonest place I could find that I could add the check without doing any major rewrites.

The purpose of this check is to make sure the functions havne't been redirected outside of the normal kernel internal redirect systems, because if they have been, we'll run into problems down the road.

…uction

This patch makes kpatch_link_object run through all the verify steps before
it attempts to patch them. On failure, there is no need to call unlink
(although unlink is called on the return path anyway - but I'll tackle that
problem another day) so add the err_verify jump point.

We also now check the start of the symbol address for the 0xe9 OP_JMP_REL32
instruction. This may not be the best place to do it, but it's the soonest
place I could find that I could add the check without doing any major
rewrites.
@cormander
Copy link
Contributor Author

Hi guys,

An easy way to test the issue I'm solving in this patch is to build / install this module;

https://github.com/cormander/tpe-lkm

And then try to load a patch that modifies one of the hijacked functions;
do_mmap_pgoff
security_bprm_check
security_file_mprotect
proc_sys_write
pid_revalidate
m_show
kallsyms_open
security_task_fix_setuid

[ 72.295961] WARNING: at kernel/trace/ftrace.c:1663 ftrace_bug+0x25d/0x270()

[ 72.296059] ftrace failed to modify [] security_bprm_check+0x0/0x30

It's not that there is a bug in ftrace (or maybe ftrace ought to do this check instead of kpatch?) but there's a lot of stacktrace under this condition. My ultimate goal would be to have kpatch do the symbol checks / etc on all symbols in all changing objects before it attempts to load them; that way, when we encounter an error, we don't run into issues backing them out because they never went in in the first place. Checking for the jump at the symbol start address would be one such of those sanity checks.

I don't expect you to take this patch verbatim - I moved the call to kpatch_write_relocations() and I'm not sure if it needs to be called before the verification steps. I don't think so, but I haven't looked at this project's internals long enough to know for sure.

@jpoimboe
Copy link
Member

jpoimboe commented Sep 2, 2014

This patch makes kpatch_link_object run through all the verify steps before it attempts to patch them. On failure, there is no need to call unlink (although unlink is called on the return path anyway - but I'll tackle that problem another day) so add the err_verify jump point.

Yeah, looks like the kpatch_link_object() error path is messed up. I'll open another issue for that.

We also now check the start of the symbol address for the 0xe9 OP_JMP_REL32 instruction. This may not be the best place to do it, but it's the soonest place I could find that I could add the check without doing any major rewrites.

I feel the same here as I do about kpatch_enabled. I'd much rather have ftrace properly return an error, instead of having kpatch be responsible for ftrace's error handling.

@jpoimboe
Copy link
Member

jpoimboe commented Sep 2, 2014

Closing for now, see #385 for the current ftrace error handling plan.

@jpoimboe jpoimboe closed this Sep 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants